Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix][admin] Fix exception thrown in getMessageId method #23784

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

danpi
Copy link
Contributor

@danpi danpi commented Dec 26, 2024

Fixes #23766

Motivation

During the fix of the previous issue, a detail was overlooked, which could lead to the exception being overwritten.

Modifications

Add a return statement after handling the LedgerNotExistException in the readEntryFailed method to ensure that the exception is not overwritten by subsequent code.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 26, 2024
@lhotari
Copy link
Member

lhotari commented Jan 3, 2025

Add a return statement after handling the LedgerNotExistException in the readEntryFailed method to ensure that the exception is not overwritten by subsequent code.

@danpi The result of a CompletableFuture doesn't change after it has been set once with complete or completeExceptionally. That's why this problem doesn't occur.

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change isn't strictly needed since CompleteFuture state doesn't change after completion. A second call to completeExceptionally will be a no-op. However, this code change is fine.

@codecov-commenter
Copy link

codecov-commenter commented Jan 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.91%. Comparing base (bbc6224) to head (0d6a87b).
Report is 832 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23784      +/-   ##
============================================
+ Coverage     73.57%   73.91%   +0.33%     
+ Complexity    32624    31813     -811     
============================================
  Files          1877     1853      -24     
  Lines        139502   146748    +7246     
  Branches      15299    17184    +1885     
============================================
+ Hits         102638   108468    +5830     
- Misses        28908    29672     +764     
- Partials       7956     8608     +652     
Flag Coverage Δ
inttests 27.54% <0.00%> (+2.95%) ⬆️
systests 23.18% <0.00%> (-1.15%) ⬇️
unittests 73.38% <100.00%> (+0.53%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...pulsar/broker/admin/impl/PersistentTopicsBase.java 69.60% <100.00%> (+4.15%) ⬆️

... and 1027 files with indirect coverage changes

@lhotari lhotari merged commit 1f7a79f into apache:master Jan 3, 2025
101 of 105 checks passed
lhotari pushed a commit that referenced this pull request Jan 3, 2025
lhotari pushed a commit that referenced this pull request Jan 3, 2025
lhotari pushed a commit that referenced this pull request Jan 3, 2025
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jan 7, 2025
Co-authored-by: houbonan <[email protected]>
(cherry picked from commit 1f7a79f)
(cherry picked from commit b738d14)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jan 8, 2025
Co-authored-by: houbonan <[email protected]>
(cherry picked from commit 1f7a79f)
(cherry picked from commit b738d14)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants